Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style hr transition in base-navigation-and-footer #15194

Closed
wants to merge 1 commit into from

Conversation

janbrasna
Copy link
Contributor

@janbrasna janbrasna commented Sep 19, 2024

One-line summary

In #15125 the hr divider was moved from refreshed pages to the refreshed footer, applied to all pre-existing pages — thus needs the styling applied independently with the footer, too.

Significant changes and points to review

Issue / Bugzilla link

Fixes #15152 and #15159

Testing

http://localhost:8000/en-CA/impact/
http://localhost:8000/en-CA/advertising/
http://localhost:8000/en-GB/firefox/developer/
http://localhost:8000/en-GB/firefox/nightly/firstrun/
http://localhost:8000/en-US/careers/

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.88%. Comparing base (bc4773b) to head (5ac415a).
Report is 180 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15194   +/-   ##
=======================================
  Coverage   77.88%   77.88%           
=======================================
  Files         162      163    +1     
  Lines        8473     8480    +7     
=======================================
+ Hits         6599     6605    +6     
- Misses       1874     1875    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +17 to 18
@import 'transition';
@import 'components/footer-refresh';
Copy link
Contributor Author

@janbrasna janbrasna Sep 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmh while working this produces not so nice results for some pages that end with colour shapes, like:

Screenshot 2024-09-22 at 17 43 03

(barely visible, but it's there…)

Screenshot 2024-09-22 at 19 10 42

or better:

Screenshot 2024-09-22 at 19 10 59

and

Screenshot 2024-09-22 at 17 45 17

or

Screenshot 2024-09-22 at 19 24 32

Hiding the hr for now with low specificity .m24-c-transition * { display: none; } (that's only effective until the proper declaration is loaded) looks perhaps better — but that doesn't solve the issue once 2024 styles are turned on site-wide — when that divider gets styled globally and places with somewhat rectangular blocks ending the pages will display like this anyway…

@janbrasna janbrasna marked this pull request as ready for review September 22, 2024 17:20
@reemhamz
Copy link
Contributor

Hmm, I'm curious on @stephaniehobson's take on this solution she suggested. It seems like this pixelated footer is going to look very awkward with some pages (such as the /advertising page screenshotted above). I know some pages are going to have their layouts updated with the new design eventually, so we could tackle those awkward spots then and move some content around, however we'll need to accept the awkwardness till then.

@stephaniehobson stephaniehobson added Needs Review Awaiting code review Frontend HTML, CSS, JS... client side stuff Refresh 🦖 Work related to the 2024 site refresh labels Oct 8, 2024
@janbrasna janbrasna closed this Nov 12, 2024
@janbrasna janbrasna deleted the fix/footer-hr-transition branch November 12, 2024 17:40
@janbrasna
Copy link
Contributor Author

This is no longer relevant as the new direction was set in #15410

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frontend HTML, CSS, JS... client side stuff Needs Review Awaiting code review Refresh 🦖 Work related to the 2024 site refresh
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New MVP footer has <hr> element adding extra space
3 participants